Skip to content

Update README#805

Merged
jayantsing-db merged 2 commits into
mainfrom
jayantsing-db/update-readme
Apr 28, 2025
Merged

Update README#805
jayantsing-db merged 2 commits into
mainfrom
jayantsing-db/update-readme

Conversation

@jayantsing-db
Copy link
Copy Markdown
Collaborator

@jayantsing-db jayantsing-db commented Apr 23, 2025

Description

This pull request updates the README of the repository. The documentation for integration tests and logging has been relocated to separate files:

  • docs/TESTING.md
  • docs/LOGGING.md

Testing

Additional Notes to the Reviewer

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even OAuthDiscoveryURL is optional here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add all auth methods here or just the top ones and reserve the uncommon ones like jwt, oauth refresh etc for the documentation page?

imo, this README is more developer facing than customer facing so we should write it so..

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to this, let's try to minimize duplication between the readme and official documentation and also add a link to the official documentation in this readme

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't mention these in our public documentation.

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this in azure MSI doesn't make sense

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add our public documentation link here for more details.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add public documentation link for config properties if one wants to go into more details on Auth

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that our public documentation doesn't include all authentication types, and we also plan to exclude them from the README. Therefore, I believe this comment is now unnecessary.

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

@gopalldb gopalldb Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also add JVM property for nio under using the driver section

--add-opens=java.base/java.nio=org.apache.arrow.memory.core ALL-UNNAMED

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown
Collaborator

@gopalldb gopalldb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update PR desc

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not add this here. We can link to public documentation page for more Auth configuration options.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we don't mention JWT in our public documentation. Do you still want me to remove this from README? [Removing for now]

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one as well. These are advanced OAuth options.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we don't mention OAuth with Refresh Token in our public documentation. Do you still want me to remove this from README? [Removing for now]

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip discoveryUrl

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip Azure MSI

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions
Copy link
Copy Markdown

Please ensure that the NEXT_CHANGELOG.md file is updated with any relevant changes.
If this is not necessary for your PR, please include the following in your PR description:
NO_CHANGELOG=true
and rerun the job.

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this cover running fake service tests also?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For fake service tests, I have added a separate detailed page in docs folder

Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this covered anywhere? else we can still keep the running part and move full documentation to a confluence page

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a separate page for integration test in docs folder

@jayantsing-db jayantsing-db merged commit 51adaab into databricks:main Apr 28, 2025
14 of 16 checks passed
@jayantsing-db jayantsing-db deleted the jayantsing-db/update-readme branch April 28, 2025 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants